-
-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add discrete saving feature to ODESolution #645
feat: add discrete saving feature to ODESolution #645
Conversation
src/solutions/solution_interface.jl
Outdated
return ps | ||
end | ||
t = A.t[tidx] | ||
idx = searchsortedfirst(discretes.t, t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searchsortedfirst
returns the first index greater than or equivalent to t
, that means that the case where there is an exact match t
has to be handled separately, since then you should return discretes.u[idx]
instead of discretes.u[idx - 1]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! I'll fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1533e0f
src/solutions/solution_interface.jl
Outdated
t = A.t[tidx] | ||
idx = searchsortedfirst(discretes.t, t) | ||
if idx == firstindex(discretes.t) | ||
error("This should never happen: there is no discrete parameter value before the current time") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same for this error, I should be able to get the value exactly at the first index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1533e0f
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #645 +/- ##
==========================================
- Coverage 41.53% 0.00% -41.54%
==========================================
Files 55 55
Lines 4582 4591 +9
==========================================
- Hits 1903 0 -1903
- Misses 2679 4591 +1912 ☔ View full report in Codecov by Sentry. |
7df7619
to
7f6658d
Compare
a40a2a3
to
7f57ef0
Compare
7f57ef0
to
1e62b87
Compare
Requires SciML/RecursiveArrayTools.jl#372 to merge and SciML/DiffEqBase.jl#1019 to work |
2eb3a80
to
b509d04
Compare
ps = parameter_values(sol) | ||
# NOTE: This is basically SII.parameter_values_at_time but that isn't public API | ||
# and once we move interpolation to SII, there's no reason for it to be | ||
if is_parameter_timeseries(sol) == Timeseries() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had expected a function with the name is_parameter_timeseries
to return a boolean, answering the question of the function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's actually a trait function necessary for dispatching which is why it returns a singleton object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, parameter_timeseries_trait
would perhaps be better?
A function starting with is
but not returning a bool is very confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that work work. I initially chose is_parameter_timeseries
because we already have is_timeseries
which (unfortunately) is also a trait function for the IsTimeseriesTrait
, which is_parameter_timeseries
also leveranges.
src/solutions/ode_solutions.jl
Outdated
masked_discs = map(sol.discretes) do disc | ||
mini = searchsortedfirst(disc.t, mint) | ||
maxi = searchsortedlast(disc.t, maxt) | ||
disc[mini:maxi] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this implementation is only correct if I
is a unit range? Should the argument type of I
be restricted so that this method cannot be called with, say, I = [1, 2, 4]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bb65e2d
ab153bd
to
ea9d214
Compare
ea9d214
to
0de0ce6
Compare
95682ad
to
1afed50
Compare
5b7e92b
to
15e6fc2
Compare
28db596
to
a9c937f
Compare
a9c937f
to
ef94a20
Compare
Co-authored-by: Fredrik Bagge Carlson <[email protected]>
ef94a20
to
fe68aaa
Compare
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.